-
Notifications
You must be signed in to change notification settings - Fork 42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: node and protocols health #2080
Conversation
size-limit report 📦
|
} | ||
|
||
private updateOverallHealth(): void { | ||
//TODO: blocked for Store by https://github.com/waku-org/js-waku/pull/2019 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps this isn't to be seen as blocked but simply that health as a metric is for LightPush and Filter only
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ Agreeing with this more. Reliability RFC is strictly LightPush + Filter
24c0840
to
ba7b44f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -232,6 +258,11 @@ export class BaseProtocolSDK implements IBaseProtocolSDK { | |||
throw error; | |||
} | |||
} | |||
|
|||
private updatePeers(peers: Peer[]): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: not to change everything else - we can introduce private set peers
and do update inside
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added comments
5329a2f
to
f761d7f
Compare
@@ -14,6 +18,7 @@ const DEFAULT_NUM_PEERS_TO_USE = 3; | |||
const DEFAULT_MAINTAIN_PEERS_INTERVAL = 30_000; | |||
|
|||
export class BaseProtocolSDK implements IBaseProtocolSDK { | |||
private healthManager: IHealthManager; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should understand if we want to support waku.lightpush.health
as well as waku.health
API
I don't have any preferences here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having it on the Waku
object, where a consumer can fetch health for both node and protocol is a good start. We can definitely expand into exposing health on the protocol SDK as well: waku.lightpush.health
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some comments and read CI, all good otherwise
f137c2c
to
9f12048
Compare
tests pass locally without an increased timeout, but fail in CI
9f12048
to
dfabf82
Compare
Problem
Based on the Reliability RFC,
Solution
Introduce node health as a metric on the Waku object:
waku.health
Also includes comprehensive tests:
BaseProtocolSDK
#2019 if requiredNotes
Contribution checklist:
!
in title if breaks public API;